feat(backup): file-hash incremental detection for heap and AO tables#98
Open
liang8283 wants to merge 2 commits into
Open
feat(backup): file-hash incremental detection for heap and AO tables#98liang8283 wants to merge 2 commits into
liang8283 wants to merge 2 commits into
Conversation
Adds two independent flags to gpbackup that let incremental backups
detect table changes by file content rather than by AO modcount alone:
--heap-file-hash
Hashes each heap table's data-file mtime+size on every segment via
pg_stat_file() (CHECKPOINT first), so unchanged heap tables can be
skipped in incremental. Default behavior (always include heap) is
preserved when the flag is absent.
--ao-file-hash
Hashes (segno, eof, tupcount) of each AO table's aoseg rows --
deliberately excluding modcount, which on GP5 propagates across
sibling partitions when only one leaf is modified. AOCS tables use
a Cloudberry-aware column set: segno + tupcount + vpinfo when
!IsGPDB(), or GP7+ schema otherwise. With the flag, partition-level
change detection works correctly even on GP5-style modcount leaks.
Both flags require --leaf-partition-data or --incremental.
Implementation:
toc/toc.go
Extends IncrementalEntries with optional Heap map[string]HeapEntry,
and AOEntry with optional FileHashMD5. Both yaml-omitempty so TOCs
written without the flags remain bytewise identical to the old
format and are readable by older binaries.
backup/queries_incremental.go
Adds ensureFileStatFunction (installs a plpgsql wrapper around
pg_stat_file in gp_toolkit; pure SQL, no plpython), getTableFileHash,
getHeapTableFQNs, GetAOContentHashes, getAOSegContentHash. Uses a
dedicated dbconn so per-table query failures do not abort the backup
transaction.
backup/wrappers.go
backupIncrementalMetadata: when --ao-file-hash is set, merges
aoseg content hashes into the existing AOEntry map; when
--heap-file-hash is set, CHECKPOINTs, then collects per-segment file
hashes for heap tables into a new HeapEntry map.
backup/incremental.go
FilterTablesForIncremental now branches independently on the two
flags. When neither is set, behavior is identical to the prior
version (AO compared by modcount+DDL, heap unconditionally included).
backup/validate.go
Rejects --*-file-hash without --leaf-partition-data or --incremental.
backup/incremental_test.go
Moves the FilterTablesForIncremental call into JustBeforeEach so the
new MustGetFlagBool reads happen after BeforeSuite initializes
cmdFlags. (Previously the call ran during spec-tree construction.)
Verification:
- make build / make lint / make unit: pass on the server
(no new lint findings; 13 ginkgo unit suites green).
- Targeted live-cluster e2e on Cloudberry 2.5.0: 4-binary install,
full + incremental cycle with both flags, mixed schema (heap/ao_row/
aocs/partitioned AO). Confirmed unchanged tables skipped, partition-
level granularity (one leaf included, sibling skipped), restore
round-trip row counts match across all tables.
- make end_to_end (1232s, 196 of 221 specs ran): no regressions
attributable to this change.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Follow-up to e24bc69. Two real bugs and two minor cleanups found in code review of that commit: 1. (data loss) gp_toolkit.gpbackup_file_info hand-built the tablespace path as 'pg_tblspc/<tsp>/<dboid>/<rfn>', missing the mandatory 'PG_<major>_<catver>' directory that PostgreSQL/Cloudberry put in the middle. pg_stat_file would fail for every heap table living in a custom tablespace; the EXCEPTION branch swallowed the error and returned the literal string '|0' (not empty). Because '|0' is not filtered by 'WHERE info <> '''', that constant became the table's hash on every backup, so any custom-tablespace heap table would appear unchanged forever and be silently skipped from every incremental backup -- losing any new rows. 2. (SQL injection / correctness) getTableFileHash interpolated FQN parts straight into the SQL via fmt.Sprintf('%s', '%s'). splitFQN stripped double quotes but left single quotes intact, so a table name with an embedded single quote (a legal pg identifier, e.g. "o'reilly") would break out of the string literal. 3. (clarity) getAOSegContentHash's default case said "GP7+ AOCS" but actually fires for GP6+. Comment fixed. 4. (consistency) HeapEntry.FileHashMD5 missing 'omitempty' that AOEntry.FileHashMD5 already had. Fix for apache#1 and apache#2 is the same change: pass the table's OID through to the plpgsql function, and let pg_relation_filepath() compute the on-disk path. The built-in already handles all tablespace layouts correctly across PG/Cloudberry versions, and oid interpolation is just an integer literal so the SQL-string-escaping concern disappears. The EXCEPTION block now returns true empty-string, so a hash failure on a single table falls through to "include in incremental" instead of poisoning the hash with a constant. Implementation notes: backup/queries_incremental.go - gp_toolkit.gpbackup_file_info now takes (p_oid oid) and uses pg_relation_filepath(p_oid). The setup path drops any older (text, text) signature first so an in-place upgrade against a previous gpbackup installation cleans up cleanly. The duplicate- check query gained 'pronargs = 1' to distinguish from the old signature. - getHeapTableFQNs -> getHeapTables, returning []heapTable{Oid,FQN}. - getTableFileHash(hashConn, oid, fqn) -- oid interpolated as integer literal, fqn used only for log messages. - getFileHashesForTables takes []heapTable. - splitFQN removed. backup/wrappers.go - One-line callsite update. toc/toc.go - HeapEntry.FileHashMD5 gets omitempty for symmetry with AOEntry. Verification: - make build / make unit on the server: green. - Targeted live cluster e2e (heap + ao_row + ao_column + partitioned AO, full + mutate + incremental + restore): every expected inclusion/exclusion matches, restored row counts match source. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds two independent, opt-in flags to
gpbackupthat let--incrementaldetect table-level changes by hashing physical-storage state rather than relying solely on AOmodcount:--heap-file-hash— hashes each heap table's per-segment data-file mtime+size (viapg_stat_file()after aCHECKPOINT), so unchanged heap tables can be skipped from incremental for the first time. Default behavior — always include heap — is preserved when the flag is absent.--ao-file-hash— hashes the AO/AOCS table'spg_aosegcontent rows (segno + eof + tupcount, deliberately excludingmodcount). This gives partition-leaf change detection in GP5 (where parentmodcountpropagates across sibling partitions) and catches somemodcount-doesn't-match-data edge cases. AOCS uses a Cloudberry-aware column set (vpinfowhen!IsGPDB(), otherwise the GP6+ schema).Both flags require
--leaf-partition-dataor--incremental(validated invalidate.go).Motivation
Two long-standing gaps in incremental backup:
modcountis sometimes wrong — on GP5-style storage, the parent partition'smodcountis the sum across children, so a single child write makes every sibling appear changed. This PR ports the algorithm originally developed incloudberry-fe/gpbackup-enhancedinto Apache Cloudberry-backup with conventions cleaned up and lint compliance.Approach
IncrementalEntries.AOalready exists; this PR adds an optionalHeapmap and an optionalFileHashMD5field toAOEntry, bothyaml:",omitempty". TOCs written without the new flags are bytewise identical to today's. Oldergprestorereads new TOCs without issue (extra fields ignored).dbconnso a per-table query failure doesn't abort the backup transaction. The heap path also issues aCHECKPOINTfirst to flush dirty pages, sopg_stat_filemtime/size reflect data state and not just buffer state.pg_relation_filepath(oid)rather than constructing the on-disk path manually — this avoids the very-easy-to-get-wrongpg_tblspc/<oid>/PG_<major>_<catver>/...path inside the plpgsql helper and is robust across PG/Cloudberry versions and tablespaces.FilterTablesForIncrementalbranches independently on each flag. When neither flag is set, control flow is logically identical to the previous version (AO compared bymodcount + DDL timestamp; heap unconditionally included).Files changed
toc/toc.goHeapEntry; addIncrementalEntries.HeapandAOEntry.FileHashMD5(bothomitempty).options/flag.goHEAP_FILE_HASH,AO_FILE_HASHconstants and pflag registrations.backup/queries_incremental.goensureFileStatFunction,getHeapTables,getTableFileHash,getFileHashesForTables,GetAOContentHashes,getAOSegContentHash. plpgsql helpergp_toolkit.gpbackup_file_info(p_oid oid)usespg_relation_filepath(oid) + pg_stat_file().backup/wrappers.gobackupIncrementalMetadatanow optionallyCHECKPOINTs and collects heap + AO content hashes.backup/incremental.goFilterTablesForIncrementalrewrite with independent AO/heap branches.backup/validate.go--heap-file-hash/--ao-file-hashwithout--leaf-partition-dataor--incremental.backup/incremental_test.goFilterTablesForIncrementalcall intoJustBeforeEachso package-levelcmdFlagsis initialized first.Backward compatibility
Fully compatible.
FilterTablesForIncremental.yaml:",omitempty". Old TOCs deserialize into the new structs; new TOCs without these flags serialize bytewise-identically to the old format.gp_toolkit.gpbackup_file_infois only created when--heap-file-hashis used; if a previous gpbackup version installed an older(text, text)signature, the setup path drops it first (DROP FUNCTION IF EXISTS ... (text, text)).How tested
make build: 5 binaries built cleanly.make lint: no new findings attributable to this change (pre-existing errcheck/unparam issues in unrelated files are unchanged).make unit: all 13 Ginkgo suites pass.make end_to_end: 162 / 196 active specs PASS / 0 regressions traceable to this change. The 34 failures are all environmental (gpbackmanbinary path, leftovertest_queue/test_tablespacebetween specs, and one test that creates tables withoutUSING heapon a cluster whose default access method isao_row).pg_relation_filepath()handles the tablespace path correctly — without it, custom-tablespace heap would silently break in incremental detection.The e2e scripts used for verification are reproducible; happy to commit them into
end_to_end/as a follow-up if reviewers want a regression test for the feature.Contributor's checklist
rat-check,build_and_unit_test, smoke/unit/integration/end_to_end/s3_plugin_e2e/regression/scale.gofmt/goimportsclean.Signed-off-bynot currently included; will amend if reviewers want DCO.